Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CLN: Deprecate pandas.SparseArray for pandas.arrays.SparseArray #30656

Merged
merged 14 commits into from
Jan 5, 2020
Merged

CLN: Deprecate pandas.SparseArray for pandas.arrays.SparseArray #30656

merged 14 commits into from
Jan 5, 2020

Conversation

Dr-Irv
Copy link
Contributor

@Dr-Irv Dr-Irv commented Jan 3, 2020

Change all references in code from pd.SparseArray to pd.arrays.SparseArray . Add deprecation message for pd.SparseArray

Per comment from @TomAugspurger here: #30644 (comment), this may require discussion.

@pep8speaks
Copy link

pep8speaks commented Jan 3, 2020 •
edited
Loading

Hello @Dr-Irv! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-05 15:15:43 UTC

jreback
jreback requested changes Jan 4, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. on the tests if you can simply import SparseArray at the top rather than using pd.array.SparseArray would be better. ping on green.

pandas/tests/arrays/sparse/test_arithmetics.py Outdated Show resolved Hide resolved
pandas/tests/arrays/sparse/test_array.py Outdated Show resolved Hide resolved
pandas/tests/arrays/sparse/test_combine_concat.py Show resolved Hide resolved
pandas/tests/base/test_conversion.py Outdated Show resolved Hide resolved
pandas/tests/extension/test_sparse.py Outdated Show resolved Hide resolved
@jreback jreback added Deprecate Functionality to remove in pandas Sparse Sparse Data Type labels Jan 4, 2020
@jreback jreback added this to the 1.0 milestone Jan 4, 2020
@jorisvandenbossche jorisvandenbossche mentioned this pull request Jan 4, 2020
Closed
@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jan 4, 2020

@jreback now green

jreback
jreback approved these changes Jan 4, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment for followon, @TomAugspurger ok here?

pandas/tests/reshape/test_reshape.py
@@ -45,7 +45,7 @@ def test_basic(self, sparse, dtype):
dtype=self.effective_dtype(dtype),
)
if sparse:
expected = expected.apply(pd.SparseArray, fill_value=0.0)
expected = expected.apply(pd.arrays.SparseArray, fill_value=0.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but i'd still prefer just using SparseArray and importing it at the top, a followup for this would be ok.

@jreback
Copy link
Contributor

jreback commented Jan 4, 2020

ahh turns out need to have you rebase. can you make those import changes as well.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jan 5, 2020 via email

@jreback
Copy link
Contributor

jreback commented Jan 5, 2020

@Dr-Irv can u merge master

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jan 5, 2020

@jreback I did the merge, but CI is failing when trying to compile pandas/_libs/src/ujson/python/objToJSON.c, and I didn't touch that.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jan 5, 2020

@jreback I did the merge, but CI is failing when trying to compile pandas/_libs/src/ujson/python/objToJSON.c, and I didn't touch that

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jan 5, 2020

@jreback I did the merge, but CI is failing when trying to compile pandas/_libs/src/ujson/python/objToJSON.c, and I didn't touch that

Seems to be reported in #30709 and fixed in #30710 so not sure what to do with many parallel things going on.

@jreback jreback merged commit 97bba51 into pandas-dev:master Jan 5, 2020
@jreback
Copy link
Contributor

jreback commented Jan 5, 2020

thanks @Dr-Irv very nice!

@Dr-Irv Dr-Irv deleted the deprsparse branch January 6, 2020 23:10
@Dr-Irv Dr-Irv mentioned this pull request Jan 6, 2020
Merged
5 tasks
@jorisvandenbossche
Copy link
Member

Consistency in our docs is certainly a good change, but I am not sure the deprecation of the top-level name is necessary. First, it's just historical reasons it is there (we have many things the way they are for historical reasons), and second, you can't create a sparse array as easily with a top-level function (pd.array, pd.date_range, etc) as you can for other array types.

Anyway, besides that comment, two practical comments on this PR:

  • The deprecation for older python versions is incorrect I think: you added a dummy class that will not function at all, so this will break instead of deprecate (the test should maybe try to create a sparse array, to ensure it still works).
  • I would also use a DeprecationWarning instead of a FutureWarning here first, we have time enough to change to FutureWarning later.

@jorisvandenbossche jorisvandenbossche mentioned this pull request Jan 7, 2020
Closed
@jreback
Copy link
Contributor

jreback commented Jan 7, 2020

FutureWarning is correct here - why add this extra step / burden
so -1 on DeprecationWarning

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jan 7, 2020

Anyway, besides that comment, two practical comments on this PR:

  • The deprecation for older python versions is incorrect I think: you added a dummy class that will not function at all, so this will break instead of deprecate (the test should maybe try to create a sparse array, to ensure it still works).

Yes, I just built an python 3.6 environment and see what you are saying, so I'll work on fixing that.

@mroeschke mroeschke mentioned this pull request Oct 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Deprecate Functionality to remove in pandas Sparse Sparse Data Type
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

SparseArray not in arrays module - inconsistent with IntegerArray, StringArray, etc.
5 participants
@Dr-Irv @pep8speaks @jreback @TomAugspurger @jorisvandenbossche